-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Siren GET /swaps
response prototype
#7
Conversation
"class": ["comit-action"], | ||
"fields": [ | ||
{ | ||
"name": "beta_ledger_refund_identity", "title": "Refund address on Ethereum", "type": "text", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is type actually text here?
[edit] Oh I see it's the media type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, siren defines a list of acceptable values.
"entities": [ | ||
{ | ||
"class": ["swap"], | ||
"rel": [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I think this is unneeded in our context but I am not sure I understand its use:
Defines the relationship of the sub-entity to its parent, per Web Linking (RFC5899).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can just be item
in our case.
See here: kevinswiber/siren#17 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you validate that it conforms to the schema? :)
Yes I configured CLion for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you validate that it conforms to the schema? :)
"fields": [ | ||
{ | ||
"name": "beta_ledger_refund_identity", "title": "Refund address on Ethereum", "type": "text", | ||
"class": ["address", "ethereum"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ (to enforce discussion)
@LLFourn voiced some concerns at some point that this should maybe be a single value ethereum-address
instead of two. What do you guys think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah at first I was surprised to see ["address", "ethereum"]
in the draft as I was expecting address-ethereum
.
I have no strong opinion on this. I think it makes sense to separate because at the end, when writing address-ethereum
we are creating our own format to express the fact that it is an address
on the ethereum
network.
Having it separated makes slightly more sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing that makes separation a bit tricky is, how to you use these values in automated system?
You basically have to try and match against a known set of values to figure out, which classes are present because all you have is a list of strings. The logic would be like:
if classes.contains("ethereum") && classes.contains("address") {
return metamask.getAddress()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'd probably be more nested than that:
if classes.contains("ethereum") {
if classes.contains("address") {
return metamask.getAddress()
} else if classes.contain("broadcast-transaction") {
// ...
}
}
no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why any field would ever be tagged with a class broadcast-transaction
?
Whether or not you nest it is a matter of taste I guess. I'd prefer not nesting it to keep things local and modular, i.e. detecting that a field is an ethereum address has little to do with detecting the field which represents the fee per byte on bitcoin. Nested if
statements convey that requirements are connected, whereas if they are not, I'd prefer the code to not suggest that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's list the possible values to understand what would be the best solution. I think it does not matter in any case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bonomat I don't really understand where you are going with your comment 🤔
From my understanding, class
is a way of tagging entities, fields and actions with domain-specific data. This serves the purpose of identifying and finding specific actions, fields or entities within the response.
The client would therefore:
- first take all actions in the response and look, which of them are tagged with
comit-action
- those it would render as buttons in table row for the swap
- upon clicking an action, the client can inspect the
fields
of this action and, if present, render a form for all fields - to enhance the user experience, it can search for known values in the
class
array to invoke special behaviour for some fields, like replacing the input field with anEthereumAddressTextField
(which has support for metamask)
The question is therefore how "modular" we want this detection to be:
Do we want to repeat ourselves in defining single-token classes like:
- ethereum-address
- bitcoin-address
- bitcoin-fee
- ...
or do we want to split those tags into:
- bitcoin
- ethereum
- address
- fee
- ...
and look for combinations of them.
Splitting them up would make them more composable, which might be an advantage in the future.
On the other hand, the first option has the advantage of making invalid combinations of classes impossible.
However, invalid combinations are actually not so much of a issue since the client is only going to invoke special behaviour upon known combinations, hence and invalid combination will not break clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After coding it I prefer 2 classes to more easily transfer the responsibility of getting the data:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomaseizinger :
Along the lines this summarizes what I meant:
- to enhance the user experience, it can search for known values in the
class
array to invoke special behaviour for some fields, like replacing the input field with anEthereumAddressTextField
(which has support for metamask)
...
Splitting them up would make them more composable, which might be an advantage in the future.
On the other hand, the first option has the advantage of making invalid combinations of classes impossible.
After coding it I prefer 2 classes to more easily transfer the responsibility of getting the data:
Well then let's go for this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me!
"entities": [ | ||
{ | ||
"class": ["swap"], | ||
"rel": [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can just be item
in our case.
See here: kevinswiber/siren#17 (comment)
"method": "POST", | ||
"href": "/swaps/rfc003/399e8ff5-9729-479e-aad8-49b03f8fc5d5/accept", | ||
"type": "application/json", | ||
"class": ["comit-action"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @bonomat this is useless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually not useless at all!
You need a way of identifying, which actions should be rendered as buttons to the user. Assuming that all actions of a swap go in the same list of choices is a bit dangerous IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand.
Can you please provide an example of 2 different actions where the sole differentiator for the API client is that one is a "class": ["comit-action"]
and the other is something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is about forward compatibility. If we don't provide the class comit-action
, clients will default to rendering all the actions of a swap next to each other. Currently, this is what we want. My thinking was that there may be SIREN actions at some point that are of a different kind other than the ones we currently have and thus we may want handle them differently from the current ones (render them in a different place for example).
Without a comit-action
class, those will have to be explicitly filtered out whereas, with a comit-action
class, the code that renders the current actions will always work, regardless of what we do in the future.
You could argue that we are never going to add any actions that need to be treated differently. Maybe that is the case, maybe not.
Thinking about it, it is probably too much premature optimization :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point and I don't think I have a strong opinion anymore :)
I am thinking that maybe a future action could be to do a 'POST' request on something else than the comit_node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomaseizinger : I'm not 100% sure I understand your thoughts:
if the client receives a POST action with some fields
, then it should know that a form is required to collect the data (if it is an interactive client).
I assume this should always be like that for all actions with some fields
.
If there are no fields, then it's a simple button (e.g. the decline action).
The way how I would interpret this is: if it is a comit-action
render a comit-logo
on top of the form/button/etc, if ethereum-action
, render an ethereum-logo
...
--> Happy to put it there for now .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomaseizinger : I'm not 100% sure I understand your thoughts:
if the client receives a POST action with somefields
, then it should know that a form is required to collect the data (if it is an interactive client).
I assume this should always be like that for all actions with somefields
.
If there are no fields, then it's a simple button (e.g. the decline action).The way how I would interpret this is: if it is a
comit-action
render acomit-logo
on top of the form/button/etc, ifethereum-action
, render anethereum-logo
...--> Happy to put it there for now .
Best to hash it out face-to-face?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed it offline - happy to remove comit-action
.
GET /swaps
response prototype
"entities": [ | ||
{ | ||
"class": ["swap"], | ||
"rel": ["item"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this point to? What is "item" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have a look at the previous discussion in this PR:
#7 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! :)
Siren prototype.
Fixes comit-network/comit-rs#917